Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for pattern matching in docker:stop and docker:remove #1215

Merged
merged 3 commits into from
Aug 10, 2019

Conversation

wrosenuance
Copy link
Contributor

This is a replacement for PR #900 that builds on the pattern matching implemented for the docker:build loadNamePattern, described in PR #1207 .

Pattern Lists

To try to keep the number of new configuration options to a minimum, the PR allows a single pattern string to be composed of several patterns by separating them with commas. Given commas are not allowed in
Docker names, no special quoting is supported to avoid splitting at a particular comma.

Since patterns could be applied to different parts of the Docker object model -- e.g. filter containers by container name or the running image name -- the list of patterns allows each pattern to be limited to a specific field, or left unnamed, in which case it applies everywhere. This means that by default, a docker:stop pattern will apply to both the container name and the image name, and if either matches, the container is stopped. To have a pattern only apply to the container name, prefix with name=, and for image name, with image=.

Global and per-image configuration

The new parameters are stopNamePattern (used by docker:stop) and removeNamePattern (used by docker:remove) that can be set globally or per-image, with per-image completely replacing any global setting. Given the pattern list targetting above it is possible that these separate stop and remove parameters could be merged into a single cleanupNamePattern, or just namePattern, but the latter then may be confusing next to other parameters like containerNamePattern (which is used for generating) and loadNamePattern.

Removing stopped containers

Like PR #900, this PR changes the behaviour of docker:stop to consider already stopped containers, because the docker:stop goal is the only goal that can remove a container, and should support removing stopped containers as well as running ones. Removing of stopped containers is only enabled if keepContainers is false.

Removing by alias regex

This is not supported because I think the removal happens by container name or image name, and a change in alias is not relevant. The change in alias might affect the behaviour of an image configuration filter (e.g. -Dfilter=), but that's not being wildcarded.

Documentation

Still absent 😅 - so this is still WIP - but I think the functionality and test cases are basically sound and in a state to review.

@wrosenuance
Copy link
Contributor Author

wrosenuance commented Apr 20, 2019

I realise now this is based on a bunch of unmerged changes from other PRs, so the best thing to review is just the final commit.

@rhuss
Copy link
Collaborator

rhuss commented Apr 21, 2019

Thanks for the PR ! Let's postpone for the next release, as I want to cut a release today.

Could you please fix the conflicts ? I will do a review then afterwards.

@wrosenuance
Copy link
Contributor Author

I rebased onto the updated master - it's much easier to follow now.

Somehow I seem to have changed the CI comments and the ones from the main project don't run anymore, just the ones I tried to configure for branches in my fork. I hope they are still running somewhere for you.

@wrosenuance wrosenuance force-pushed the feature-clean-mojo branch 2 times, most recently from 0fc1f3e to dc486e3 Compare April 25, 2019 03:42
@wrosenuance
Copy link
Contributor Author

Hi @rhuss, I have been tied up for a couple of weeks, but I would like to get this PR ready to merge.

I think the main thing that is lacking is documentation. Did you have any other feedback on the proposal?

@rhuss
Copy link
Collaborator

rhuss commented May 17, 2019

@wrosenuance sorry, same for me. I only can work in bursts on this plugin. I will have a look when flying back home. Sorry for the delay.

Separate name patterns are supported for each goal. For docker:stop, the patterns are applied to the container name and container image repo tag. While for docker:remove, the patterns are applied to the image repo tags.

Signed-off-by: William Rose <william.rose@nuance.com>
Signed-off-by: William Rose <william.rose@nuance.com>
wrosenuance added a commit to wrosenuance/docker-maven-plugin that referenced this pull request Aug 9, 2019
@wrosenuance
Copy link
Contributor Author

Hi @rhuss, I saw your comment on the other PR about being away for a few weeks and I thought I should try to get this in before you head out!

I wrote up the documentation, and revised some of the behavior to hopefully make more sense.

The main change is that where the stopNamePattern and removeNamePattern on the mojos was previously used as a fallback if the image did not define one, they are now used to perform a pattern-based stop or remove independently of any defined images.

This is because I think there are two likely use cases for pattern-based stop/remove:

  • remove everything - uses the mojo-level configuration in an execution that defines no images
  • remove instances related to a particular image - uses the image-level configuration

In the scenario where you want to stop/remove using per-image patterns, I can't see it is much help to have a mojo-level fallback.

@rhuss
Copy link
Collaborator

rhuss commented Aug 9, 2019

thanks for all your patience (and apologies for being late again). I try to squeeze that PR in today, so let's get that done. I would then make also a new release before going to vacations ;-)

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for all your work. On a quick glance it looks really awesome. So let's get that merge and if there any issue we can fix them later.

I'm going to make a release this weekend, too.

thanks !

@rhuss rhuss merged commit 4d3714f into fabric8io:master Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants